xen: arm: avoid truncation in mfn to paddr conversions

Although MFNs are 64-bit in the hypercall ABI they are most often unsigned
long internally, and therefore be 32-bit on arm32. Physical addresses are
always 64-bit via paddr_t.

This means that the common "mfn << PAGE_SHIFT" pattern risks losing some of
the top bits of the address is high enough. This need not imply a high amount
of RAM, just a sparse physical address map.

The correct form is ((paddr_t)mfn)<<PAGE_SHIFT and we have the pfn_to_paddr
macro which implements this. Grep for PAGE_SHIFT and << and switch to the
macro everywhere we can in the arch specific code. Note that page.h is
included by mm.h which defines the macro and so remains with the open coded
cast. I have inspected the common code matching this pattern and it uses the
correct casts where necessary (x86 also has pfn_to_paddr, so as a further
cleanup we could fix the common code too, but I haven't done that here).

I observed this as failure to boot a guest on midway, due to trying to map a
foreign page which belonged to no guest. I think this likely explains the
crashes which Julien has seen too.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 99e785aaaab3ba7037affbbd910bfab8778c61b0..7c855cbc7deee16335250f2f77f5a9f00dc54ede 100644 (file)
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -84,8 +84,8 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
         panic("Failed to allocate contiguous memory for dom0\n");
 
     spfn = page_to_mfn(pg);
-    start = spfn << PAGE_SHIFT;
-    size = (1 << order) << PAGE_SHIFT;
+    start = pfn_to_paddr(spfn);
+    size = pfn_to_paddr((1 << order));
 
     // 1:1 mapping
     printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0)\n",

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 2de7dc7d0fce6155e99cd9eb3dae373a946eb9dd..81899151324641eb26602ed3b2c9c463924ad454 100644 (file)
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1032,10 +1032,10 @@ static int xenmem_add_to_physmap_one(
             return rc;
         }
 
-        maddr = p2m_lookup(od, idx << PAGE_SHIFT);
+        maddr = p2m_lookup(od, pfn_to_paddr(idx));
         if ( maddr == INVALID_PADDR )
         {
-            dump_p2m_lookup(od, idx << PAGE_SHIFT);
+            dump_p2m_lookup(od, pfn_to_paddr(idx));
             rcu_unlock_domain(od);
             return -EINVAL;
         }

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index af32511f560f03e97f904dc6168424cddd56771c..1d5c8410148ab9e4c5e0dcd7b1794ad3287477e7 100644 (file)
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -296,18 +296,20 @@ int guest_physmap_add_page(struct domain *d,
                            unsigned long mfn,
                            unsigned int page_order)
 {
-    return create_p2m_entries(d, INSERT, gpfn << PAGE_SHIFT,
-                              (gpfn + (1<<page_order)) << PAGE_SHIFT,
-                              mfn << PAGE_SHIFT, MATTR_MEM);
+    return create_p2m_entries(d, INSERT,
+                              pfn_to_paddr(gpfn),
+                              pfn_to_paddr(gpfn + (1<<page_order)),
+                              pfn_to_paddr(mfn), MATTR_MEM);
 }
 
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order)
 {
-    create_p2m_entries(d, REMOVE, gpfn << PAGE_SHIFT,
-                       (gpfn + (1<<page_order)) << PAGE_SHIFT,
-                       mfn << PAGE_SHIFT, MATTR_MEM);
+    create_p2m_entries(d, REMOVE,
+                       pfn_to_paddr(gpfn),
+                       pfn_to_paddr(gpfn + (1<<page_order)),
+                       pfn_to_paddr(mfn), MATTR_MEM);
 }
 
 int p2m_alloc_table(struct domain *d)
@@ -450,7 +452,7 @@ err:
 
 unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
 {
-    paddr_t p = p2m_lookup(d, gpfn << PAGE_SHIFT);
+    paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn));
     return p >> PAGE_SHIFT;
 }
 

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d252131e1a2c4679ad0d5e6dfff6004b0fad5482..0795eb9a643479deeb66a6f0acc129989d4f4bcd 100644 (file)
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -409,7 +409,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
 
     do
     {
-        e = consider_modules(ram_start, ram_end, xenheap_pages<<PAGE_SHIFT,
+        e = consider_modules(ram_start, ram_end,
+                             pfn_to_paddr(xenheap_pages),
                              32<<20, 0);
         if ( e )
             break;
@@ -423,7 +424,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     domheap_pages = heap_pages - xenheap_pages;
 
     early_printk("Xen heap: %"PRIpaddr"-%"PRIpaddr" (%lu pages)\n",
-                 e - (xenheap_pages << PAGE_SHIFT), e,
+                 e - (pfn_to_paddr(xenheap_pages)), e,
                  xenheap_pages);
     early_printk("Dom heap: %lu pages\n", domheap_pages);
 
@@ -471,8 +472,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
             e = ram_end;
 
         /* Avoid the xenheap */
-        if ( s < ((xenheap_mfn_start+xenheap_pages) << PAGE_SHIFT)
-             && (xenheap_mfn_start << PAGE_SHIFT) < e )
+        if ( s < pfn_to_paddr(xenheap_mfn_start+xenheap_pages)
+             && pfn_to_paddr(xenheap_mfn_start) < e )
         {
             e = pfn_to_paddr(xenheap_mfn_start);
             n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ce66099d265ab813226f1b680059c6aa759ec4f6..b8d4e7d4fa7a73dc7a678e0df7de1f686a280d8f 100644 (file)
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -123,8 +123,8 @@ extern unsigned long xenheap_virt_end;
 #endif
 
 #define is_xen_fixed_mfn(mfn)                                   \
-    ((((mfn) << PAGE_SHIFT) >= virt_to_maddr(&_start)) &&       \
-     (((mfn) << PAGE_SHIFT) <= virt_to_maddr(&_end)))
+    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
+     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
 
 #define page_get_owner(_p)    (_p)->v.inuse.domain
 #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))